fix: pass golangci-lint 2.11+ checks#307
Conversation
Move defer cancel() inside goroutines so each goroutine owns its context lifecycle. This satisfies gosec G118 (cancel not called) without triggering gocritic deferInLoop. In iptables/loss.go, also fix a pre-existing closure capture bug: ctx was reassigned each iteration with = but captured by closure, so all goroutines shared the last value. Switch to per-iteration iptCtx with := (matching the netem pattern) to give each goroutine its own context derived from the original ctx. Remove stale //nolint:revive on pkg/util/util.go. Closes #306
Test Results546 tests 544 ✅ 2s ⏱️ Results for commit 1eab1f6. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #307 +/- ##
==========================================
- Coverage 71.91% 71.45% -0.46%
==========================================
Files 46 46
Lines 2051 2018 -33
==========================================
- Hits 1475 1442 -33
Misses 576 576 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Updates several chaos commands to satisfy newer golangci-lint (gosec G118 / nolintlint) checks by ensuring each derived context’s cancel function is invoked in a way linters can trace.
Changes:
- Remove stale
//nolint:revivedirective frompkg/util/util.go. - Refactor netem/iptables chaos commands to call
cancel()inside each goroutine instead of collecting cancel funcs and deferring a cleanup loop. - Adjust iptables loss command to avoid reassigning the parent
ctxin the loop.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/util.go | Removes unused //nolint:revive directive. |
| pkg/chaos/netem/rate.go | Moves context canceling into goroutine for rate injection. |
| pkg/chaos/netem/loss_state.go | Moves context canceling into goroutine for 4-state loss injection. |
| pkg/chaos/netem/loss_ge.go | Moves context canceling into goroutine for GE-model loss injection. |
| pkg/chaos/netem/loss.go | Moves context canceling into goroutine for loss injection. |
| pkg/chaos/netem/duplicate.go | Moves context canceling into goroutine for duplicate injection. |
| pkg/chaos/netem/delay.go | Moves context canceling into goroutine for delay injection. |
| pkg/chaos/netem/corrupt.go | Moves context canceling into goroutine for corruption injection. |
| pkg/chaos/iptables/loss.go | Uses a per-iteration derived context and cancels it in the goroutine. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| netemCtx, cancel := context.WithTimeout(ctx, n.duration) | ||
| cancels[i] = cancel | ||
| wg.Add(1) | ||
| go func(i int, c *container.Container) { | ||
| defer wg.Done() | ||
| defer cancel() |
There was a problem hiding this comment.
This is incorrect. All these files use := (short variable declaration), not = (assignment). In Go, := inside a for loop body creates new block-scoped variables per iteration — each goroutine closure captures its own distinct netemCtx/cancel (or iptCtx/cancel). There is no sharing across iterations.
The old iptables/loss.go code did have this bug (it used ctx, cancel = ... with =), which this PR fixes by switching to iptCtx, cancel := ... with :=.
| iptCtx, cancel := context.WithTimeout(ctx, n.duration) | ||
| wg.Add(1) | ||
| go func(i int, c *container.Container) { | ||
| defer wg.Done() | ||
| errs[i] = runIPTables(ctx, n.client, c, addCmdPrefix, delCmdPrefix, cmdSuffix, n.srcIPs, n.dstIPs, n.sports, n.dports, n.duration, n.image, n.pull, n.dryRun) | ||
| defer cancel() |
There was a problem hiding this comment.
This is incorrect. All these files use := (short variable declaration), not = (assignment). In Go, := inside a for loop body creates new block-scoped variables per iteration — each goroutine closure captures its own distinct netemCtx/cancel (or iptCtx/cancel). There is no sharing across iterations.
The old iptables/loss.go code did have this bug (it used ctx, cancel = ... with =), which this PR fixes by switching to iptCtx, cancel := ... with :=.
| netemCtx, cancel := context.WithTimeout(ctx, n.duration) | ||
| cancels[i] = cancel | ||
| wg.Add(1) | ||
| go func(i int, c *container.Container) { | ||
| defer wg.Done() | ||
| defer cancel() |
There was a problem hiding this comment.
This is incorrect. All these files use := (short variable declaration), not = (assignment). In Go, := inside a for loop body creates new block-scoped variables per iteration — each goroutine closure captures its own distinct netemCtx/cancel (or iptCtx/cancel). There is no sharing across iterations.
The old iptables/loss.go code did have this bug (it used ctx, cancel = ... with =), which this PR fixes by switching to iptCtx, cancel := ... with :=.
| netemCtx, cancel := context.WithTimeout(ctx, n.duration) | ||
| cancels[i] = cancel | ||
| wg.Add(1) | ||
| go func(i int, c *container.Container) { | ||
| defer wg.Done() | ||
| defer cancel() |
There was a problem hiding this comment.
This is incorrect. All these files use := (short variable declaration), not = (assignment). In Go, := inside a for loop body creates new block-scoped variables per iteration — each goroutine closure captures its own distinct netemCtx/cancel (or iptCtx/cancel). There is no sharing across iterations.
The old iptables/loss.go code did have this bug (it used ctx, cancel = ... with =), which this PR fixes by switching to iptCtx, cancel := ... with :=.
| netemCtx, cancel := context.WithTimeout(ctx, n.duration) | ||
| cancels[i] = cancel | ||
| wg.Add(1) | ||
| go func(i int, c *container.Container) { | ||
| defer wg.Done() | ||
| defer cancel() |
There was a problem hiding this comment.
This is incorrect. All these files use := (short variable declaration), not = (assignment). In Go, := inside a for loop body creates new block-scoped variables per iteration — each goroutine closure captures its own distinct netemCtx/cancel (or iptCtx/cancel). There is no sharing across iterations.
The old iptables/loss.go code did have this bug (it used ctx, cancel = ... with =), which this PR fixes by switching to iptCtx, cancel := ... with :=.
| netemCtx, cancel := context.WithTimeout(ctx, n.duration) | ||
| cancels[i] = cancel | ||
| wg.Add(1) | ||
| go func(i int, c *container.Container) { | ||
| defer wg.Done() | ||
| defer cancel() |
There was a problem hiding this comment.
This is incorrect. All these files use := (short variable declaration), not = (assignment). In Go, := inside a for loop body creates new block-scoped variables per iteration — each goroutine closure captures its own distinct netemCtx/cancel (or iptCtx/cancel). There is no sharing across iterations.
The old iptables/loss.go code did have this bug (it used ctx, cancel = ... with =), which this PR fixes by switching to iptCtx, cancel := ... with :=.
| netemCtx, cancel := context.WithCancel(ctx) | ||
| cancels[i] = cancel | ||
| wg.Add(1) | ||
| go func(i int, c *container.Container) { | ||
| defer wg.Done() | ||
| defer cancel() |
There was a problem hiding this comment.
This is incorrect. All these files use := (short variable declaration), not = (assignment). In Go, := inside a for loop body creates new block-scoped variables per iteration — each goroutine closure captures its own distinct netemCtx/cancel (or iptCtx/cancel). There is no sharing across iterations.
The old iptables/loss.go code did have this bug (it used ctx, cancel = ... with =), which this PR fixes by switching to iptCtx, cancel := ... with :=.
| netemCtx, cancel := context.WithTimeout(ctx, n.duration) | ||
| cancels[i] = cancel | ||
| wg.Add(1) | ||
| go func(i int, c *container.Container) { | ||
| defer wg.Done() | ||
| defer cancel() |
There was a problem hiding this comment.
This is incorrect. All these files use := (short variable declaration), not = (assignment). In Go, := inside a for loop body creates new block-scoped variables per iteration — each goroutine closure captures its own distinct netemCtx/cancel (or iptCtx/cancel). There is no sharing across iterations.
The old iptables/loss.go code did have this bug (it used ctx, cancel = ... with =), which this PR fixes by switching to iptCtx, cancel := ... with :=.
Summary
defer cancel()inside goroutines — each goroutine owns its context lifecycleiptables/loss.gowherectxwas reassigned with=each iteration but captured by closure, causing all goroutines to share the last context value. Switched to per-iterationiptCtxwith:=(matching the netem pattern)//nolint:revivedirective onpkg/util/util.goCloses #306
Details
golangci-lint 2.11+ introduced stricter gosec G118 checking that flags
context.WithTimeout/context.WithCancelwhen the cancel function isn't directly called or deferred. The old pattern collected cancels in a slice and deferred a cleanup loop afterwg.Wait()— gosec couldn't trace through the slice.The naive fix (
defer cancel()in the loop body) triggers gocritic'sdeferInLoop. The correct idiomatic Go fix isdefer cancel()inside the goroutine's function literal, where:Test plan
make lint— 0 issuesmake test(via pre-commit hook) — all tests pass